pkg/controllers/kube-apiserver.go (1)

542-557: Function name is misleading.

restartMicroshiftEtcdScope only stops the scope; the restart occurs when MicroShift itself restarts after Run() returns an error. Consider renaming to stopMicroshiftEtcdScope for clarity.

♻️ Suggested rename

-// restartMicroshiftEtcdScope restarts the microshift-etcd.scope to recover from deadlock.
-// This forces a clean restart of etcd which can help break the circular dependency.
-func restartMicroshiftEtcdScope() error {
+// stopMicroshiftEtcdScope stops the microshift-etcd.scope to recover from deadlock.
+// MicroShift will restart both etcd and kube-apiserver after Run() returns an error.
+func stopMicroshiftEtcdScope() error {
 	klog.Info("Stopping microshift-etcd.scope for recovery")

Also update the call site at line 438.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/controllers/kube-apiserver.go` around lines 542 - 557, The function
restartMicroshiftEtcdScope is misnamed because it only stops the scope; rename
the function to stopMicroshiftEtcdScope (and update its comment and klog
messages accordingly) and update every call site that invokes
restartMicroshiftEtcdScope to call stopMicroshiftEtcdScope instead; ensure
exported/internal visibility remains unchanged and run tests/build to verify no
references remain to the old name.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

pkg/controllers/kube-apiserver.go (1)

468-515: ⚠️ Potential issue | 🟠 Major

Wall-clock deadline break still triggers false deadlock signal.

When rbacHookMaxWaitDuration expires at line 472, the loop breaks and falls through to close(deadlockDetected) at line 515—even if etcd was never confirmed healthy. The "etcd healthy + hook stuck" predicate may never have been observed.

Additionally, checkCount++ at line 481 executes before conditions are verified. If isEtcdHealthy errors, the count advances without observing the deadlock predicate.

Proposed fix

+	deadlockPredicateObserved := false
+
 	for checkCount < maxChecks {
 		// Check absolute deadline first - this cannot be reset by etcd state changes
 		if time.Since(startTime) >= rbacHookMaxWaitDuration {
 			klog.Errorf("RBAC bootstrap hook exceeded maximum wait duration of %v", rbacHookMaxWaitDuration)
-			break
+			if !deadlockPredicateObserved {
+				klog.Warning("Wall-clock deadline reached but deadlock predicate never confirmed - not signaling deadlock")
+				return
+			}
+			break // predicate was observed, signal deadlock
 		}
 
 		select {
 		case <-ctx.Done():
 			return
 		case <-time.After(rbacHookCheckInterval * time.Second):
 		}
 
-		checkCount++
-
 		// Check RBAC hook status
 		var status int
 		err := restClient.Get().AbsPath("/readyz/poststarthook/rbac/bootstrap-roles").Do(ctx).StatusCode(&status).Error()
 
 		// If hook is ready, no deadlock
 		if err == nil && status == 200 {
 			klog.V(4).Info("RBAC bootstrap hook completed successfully")
 			return
 		}
 
 		// Hook not ready - check if etcd is healthy
 		etcdHealthy, etcdErr := isEtcdHealthy(ctx)
 		if etcdErr != nil {
 			klog.V(4).Infof("Could not check etcd health: %v", etcdErr)
 			continue
 		}
 
 		if etcdHealthy {
+			checkCount++
+			deadlockPredicateObserved = true
 			klog.Warningf("RBAC bootstrap hook not ready (check %d/%d, elapsed %v), but etcd is healthy - potential deadlock",
 				checkCount, maxChecks, time.Since(startTime).Round(time.Second))
 		} else {
 			// etcd not healthy - not a deadlock, just waiting for etcd
 			klog.V(4).Infof("RBAC hook waiting, etcd not yet healthy (check %d/%d)", checkCount, maxChecks)
-			// Reset counter since this isn't a deadlock condition
-			// Note: wall-clock deadline (startTime) is NOT reset - flapping cannot extend indefinitely
 			checkCount = 0
 		}
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/controllers/kube-apiserver.go` around lines 468 - 515, The loop currently
can fall through to close(deadlockDetected) on wall-clock timeout even if etcd
was never observed healthy and also increments checkCount before confirming etcd
health; fix by tracking whether etcd has ever been observed healthy (e.g., add
sawEtcdHealthy bool set to true when isEtcdHealthy(ctx) returns (true, nil)),
only increment checkCount when etcdHealthy==true and etcdErr==nil (move the
checkCount++ after the etcd health success path), and after the loop only signal
deadlock (close(deadlockDetected)) if sawEtcdHealthy is true and the RBAC hook
still isn’t ready; leave the wall-clock timeout to break the loop but not
unconditionally declare a deadlock. Ensure references:
restClient.Get().AbsPath("/readyz/poststarthook/rbac/bootstrap-roles").Do(...).StatusCode(&status).Error(),
isEtcdHealthy(ctx), checkCount, sawEtcdHealthy, and deadlockDetected.

pkg/controllers/kube-apiserver.go (1)

566-568: Consider context-aware wait.

The 1s time.Sleep isn't interruptible. Low impact but a select with ctx.Done() would be cleaner for graceful shutdown.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/controllers/kube-apiserver.go` around lines 566 - 568, Replace the
uninterruptible time.Sleep(1 * time.Second) with a context-aware wait so
shutdown can be cancelled: where the sleep occurs (the time.Sleep call in
kube-apiserver.go cleanup/wait block), use a select that waits on time.After(1 *
time.Second) and also listens on ctx.Done(), returning early if ctx is
cancelled; ensure you reference the existing ctx variable and keep the same
timeout duration.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

pkg/controllers/kube-apiserver.go (2)

371-371: ⚠️ Potential issue | 🔴 Critical

Use duration constants directly.

Line 371 now overflows because kubeAPIStartupTimeout is already a time.Duration. Line 493 also turns the 2s detector interval into a multi-decade sleep. Use the constants directly and format durations with %s.

Proposed fix

-		err := wait.PollUntilContextTimeout(ctx, time.Second, kubeAPIStartupTimeout*time.Second, true, func(ctx context.Context) (bool, error) {
+		err := wait.PollUntilContextTimeout(ctx, time.Second, kubeAPIStartupTimeout, true, func(ctx context.Context) (bool, error) {
...
-		return fmt.Errorf("RBAC bootstrap hook deadlock detected after %d seconds", rbacHookDeadlockTimeout)
+		return fmt.Errorf("RBAC bootstrap hook deadlock detected after %s", rbacHookDeadlockTimeout)
...
-	case <-time.After(rbacHookCheckInterval * time.Second):
+	case <-time.After(rbacHookCheckInterval):

Verify the stale multipliers are gone:

#!/bin/bash
rg -n 'kubeAPIStartupTimeout\s*\*\s*time\.Second|rbacHookCheckInterval\s*\*\s*time\.Second|after %d seconds' pkg/controllers/kube-apiserver.go

Also applies to: 446-446, 490-494

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/controllers/kube-apiserver.go` at line 371, The code is multiplying
existing time.Duration constants (e.g. kubeAPIStartupTimeout,
rbacHookCheckInterval) by time.Second which causes overflow and wrong intervals;
update calls like wait.PollUntilContextTimeout(ctx, time.Second,
kubeAPIStartupTimeout*time.Second, ...) and any other occurrences
(rbacHookCheckInterval*time.Second) to pass the durations directly (use
kubeAPIStartupTimeout and rbacHookCheckInterval as the timeout/interval
arguments) and change any formatted log strings that use "after %d seconds" to
use "%s" with the duration value; search for usages in functions referencing
kubeAPIStartupTimeout and rbacHookCheckInterval and replace the multipliers and
integer %d format with the direct duration and %s respectively.

---

496-515: ⚠️ Potential issue | 🟠 Major

Don’t skip HTTP not-ready responses.

client-go can return both status != 0 and err != nil for HTTP error responses, so Line 513 skips the RBAC “not ready” signal the detector needs. Only skip transport/timeout failures where no HTTP status was received.

Proposed fix

-		if err != nil {
+		if err != nil && status == 0 {
 			klog.V(4).Infof("RBAC probe error (not counting toward deadlock): %v", err)
 			continue
 		}

Verify client-go Result status/error behavior in the vendored source:

#!/bin/bash
sed -n '496,515p' pkg/controllers/kube-apiserver.go
rg -n -A8 -B4 'func \(r .*Result\) StatusCode|func \(r .*Result\) Error|func \(r \*Request\) transformResponse' ./deps/github.com/openshift/kubernetes/staging/src/k8s.io/client-go/rest/request.go

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/controllers/kube-apiserver.go` around lines 496 - 515, The code currently
treats any err from restClient.Get().Do(...).Error() as a transport/timeout
failure and skips counting toward deadlock; change that logic so only
transport-level failures (err != nil AND status == 0) are skipped. In other
words, keep the probe request creation (probeCtx, cancel, restClient.Get...
StatusCode(&status).Error()), then replace the "if err != nil { ... continue }"
check with a conditional that logs and continues only when err != nil && status
== 0; if err != nil but status != 0, treat it as an HTTP error response (do not
continue) so the RBAC “not ready” HTTP status is observed by the detector
(referencing probeCtx, status, and err).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes